Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Core] publisher config API #1561

Merged
merged 27 commits into from
May 16, 2024
Merged

Conversation

rex-schilasky
Copy link
Contributor

Description

Draft implementation of a publisher configuration API in preparation of the new global eCAL configuration API.

@rex-schilasky rex-schilasky added the cherry-pick-to-NONE Don't cherry-pick these changes label Apr 23, 2024
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

ecal/core/include/ecal/msg/capnproto/publisher.h Outdated Show resolved Hide resolved
ecal/core/include/ecal/msg/publisher.h Outdated Show resolved Hide resolved
ecal/core/include/ecal/msg/publisher.h Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

ecal/tests/cpp/pubsub_test/src/pubsub_multibuffer.cpp Outdated Show resolved Hide resolved
ecal/tests/cpp/pubsub_test/src/pubsub_test.cpp Outdated Show resolved Hide resolved
ecal/tests/cpp/pubsub_test/src/pubsub_test.cpp Outdated Show resolved Hide resolved
ecal/tests/cpp/pubsub_test/src/pubsub_test.cpp Outdated Show resolved Hide resolved
ecal/tests/cpp/pubsub_test/src/pubsub_test.cpp Outdated Show resolved Hide resolved
ecal/tests/cpp/pubsub_test/src/pubsub_test.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

ecal/tests/cpp/pubsub_test/src/pubsub_test.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 20 out of 26. Check the log or trigger a new build to see more.

ecal/tests/cpp/pubsub_test/src/pubsub_test_udp.cpp Outdated Show resolved Hide resolved
ecal/tests/cpp/pubsub_test/src/pubsub_test_udp.cpp Outdated Show resolved Hide resolved
ecal/tests/cpp/pubsub_test/src/pubsub_test_udp.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

memory_file_attr.reserve = m_config.memfile_reserve_percent;
memory_file_attr.timeout_open_ms = PUB_MEMFILE_OPEN_TO;
memory_file_attr.timeout_ack_ms = m_config.acknowledge_timeout_ms;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe extract this into its own function?

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

ecal/core/include/ecal/ecal_publisher_config.h Outdated Show resolved Hide resolved
ecal/core/include/ecal/ecal_publisher_config.h Outdated Show resolved Hide resolved
ecal/core/include/ecal/ecal_publisher_config.h Outdated Show resolved Hide resolved
ecal/core/include/ecal/msg/flatbuffers/publisher.h Outdated Show resolved Hide resolved
ecal/core/include/ecal/msg/flatbuffers/publisher.h Outdated Show resolved Hide resolved
ecal/core/include/ecal/msg/publisher.h Outdated Show resolved Hide resolved
ecal/core/include/ecal/msg/publisher.h Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

ecal/core/include/ecal/msg/capnproto/publisher.h Outdated Show resolved Hide resolved
ecal/tests/cpp/pubsub_test/src/pubsub_test.cpp Outdated Show resolved Hide resolved
ecal/tests/cpp/pubsub_test/src/pubsub_test.cpp Outdated Show resolved Hide resolved
ecal/tests/cpp/pubsub_test/src/pubsub_test.cpp Outdated Show resolved Hide resolved
ecal/tests/cpp/pubsub_test/src/pubsub_test.cpp Outdated Show resolved Hide resolved
SHMConfig -> Publisher::SHM::Configuration
UDPConfig -> Publisher::UDP::Configuration
TCPConfig -> Publisher::TCP::Configuration
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

ecal/core/include/ecal/msg/publisher.h Show resolved Hide resolved
ecal/core/include/ecal/msg/publisher.h Show resolved Hide resolved
@rex-schilasky rex-schilasky marked this pull request as ready for review May 6, 2024 12:34
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

ecal/core/src/readwrite/ecal_writer.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

ecal/core/src/readwrite/ecal_writer.cpp Outdated Show resolved Hide resolved
ecal/core/src/readwrite/ecal_writer.h Show resolved Hide resolved
reader Create/Destroy replaced by Constructor/Destructor
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

ecal/core/src/readwrite/ecal_reader.cpp Outdated Show resolved Hide resolved
ecal/core/src/readwrite/ecal_reader.cpp Outdated Show resolved Hide resolved
ecal/core/src/readwrite/ecal_writer.cpp Outdated Show resolved Hide resolved
…scriber, client, server)

Create/Destroy -> Start/Stop
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

}

void CSubGate::Create()
void CSubGate::Start()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: method 'Start' can be made static [readability-convert-member-functions-to-static]

ecal/core/src/pubsub/ecal_subgate.h:42:

-     void Start();
+     static void Start();

}

void CClientGate::Create()
void CClientGate::Start()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: method 'Start' can be made static [readability-convert-member-functions-to-static]

ecal/core/src/service/ecal_clientgate.h:47:

-     void Start();
+     static void Start();

}

void CServiceGate::Create()
void CServiceGate::Start()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: method 'Start' can be made static [readability-convert-member-functions-to-static]

ecal/core/src/service/ecal_servicegate.h:41:

-     void Start();
+     static void Start();

@rex-schilasky rex-schilasky changed the title new publisher config API first draft Publisher Config API May 13, 2024
@rex-schilasky rex-schilasky changed the title Publisher Config API [Core] publisher config API May 13, 2024
Copy link
Contributor

@KerstinKeller KerstinKeller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job 😄 we're moving forward.

UDP::Configuration udp;
TCP::Configuration tcp;

bool share_topic_type = true; //!< share topic type via registration
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should discuss if we still support this for ecal 6. Also do we need to distinguish between type and descriptor? maybe only allow to switch of descriptor? (but not in this PR)

#endif
// register publisher gateway (for publisher memory file and event name)
if (m_created) return(false);
if (topic_name_.empty()) return(false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I like this 😨 BUT not for this PR.

m_use_udp_mc_confirmed(false),
m_use_shm_confirmed(false),
m_use_tcp_confirmed(false),
m_share_ttype(Config::IsTopicTypeSharingEnabled()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rex-schilasky why do you have config calls here? because we don't yet have the config on subscriber side?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, will be part of the new CSubscriber::Configuration

@@ -44,7 +44,7 @@ namespace eCAL
attr.port = UDP::GetPayloadPort();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other question: I think we indirectly access the config here? does it make sense to move also this information to the Publisher::UDP::Configuration? Although we would never want the user to change those.... (just out of curiosity, not relevant for this PR).

@rex-schilasky rex-schilasky merged commit f3312fa into master May 16, 2024
17 of 18 checks passed
@rex-schilasky rex-schilasky deleted the feature/parameter-api-publisher branch May 16, 2024 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-to-NONE Don't cherry-pick these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants